feat(secretserver): use official secretserver library#930
feat(secretserver): use official secretserver library#930yxxhero merged 1 commit intohelmfile:mainfrom
Conversation
pd-gov
requested review from
itscaro,
mumoshu,
xiaomudk and
yxxhero
as code owners
pd-gov
force-pushed
the
feature/secretserver
branch
from
23f577f to
0d0c416
Compare
|
Hm, I don't get the lint issue. The file is formatted using the vs code golang extension. |
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the SecretServer provider by migrating from a raw HTTP API implementation to the official Delinea TSS SDK (github.com/DelineaXPM/tss-sdk-go/v3). The provider identifier is changed from "secretserver" to "tss" to align with the SDK naming conventions and External Secrets Operator implementation.
Key Changes:
- Replaced custom HTTP client implementation with the official Delinea TSS SDK v3.0.1
- Changed provider identifier from "secretserver" to "tss" with corresponding environment variable updates (e.g.,
SECRETSERVER_TOKEN→TSS_TOKEN,SECRETSERVER_URL→TSS_SERVER_URL) - Enhanced secret lookup capability to support both numeric IDs and secret names, aligning with External Secrets Operator patterns
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| vals.go | Updated provider constant from "secretserver" to "tss" and simplified provider initialization to return error directly |
| pkg/stringprovider/stringprovider.go | Updated case statement from "secretserver" to "tss" to match new provider identifier |
| pkg/providers/secretserver/secretserver.go | Complete rewrite using official SDK: replaced ~100 lines of HTTP client code with ~20 lines using the TSS SDK, added support for name-based secret lookup |
| go.mod | Added dependency on github.com/DelineaXPM/tss-sdk-go/v3 v3.0.1 |
| go.sum | Added checksums for the new TSS SDK dependency |
| README.md | Updated documentation with new TSS_* environment variables, improved configuration examples for both on-prem and cloud deployments, and added limitations section |
| if field, ok := secret.Field(fieldName); ok { | ||
| return field, nil | ||
| } else { | ||
| return "", fmt.Errorf("cannot find field %s in secret %s", fieldName, secretID) | ||
| } |
There was a problem hiding this comment.
The else branch is unnecessary after a return statement. The code can be simplified by removing the else and unindenting the error return, which improves readability and follows Go best practices.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
pd-gov
force-pushed
the
feature/secretserver
branch
2 times, most recently
from
09a65a2 to
9847351
Compare
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/helmfile/vals/pkg/api" |
pd-gov
force-pushed
the
feature/secretserver
branch
3 times, most recently
from
a9c53c2 to
197c554
Compare
| return httpjson.New(l, provider), nil | ||
| case "scaleway": | ||
| return scaleway.New(l, provider), nil | ||
| case "secretserver": |
There was a problem hiding this comment.
@pd-gov why change secretserver to tss? this is a beaking change?
There was a problem hiding this comment.
I tried to align the naming scheme with that used by the vendor. I though this was Okay, since the Provider was introduced just a few days ago.
There was a problem hiding this comment.
@yxxhero I can change it back if you prefere
There was a problem hiding this comment.
@pd-gov maybe we can add alias. like : case "tss" "secretserver"
There was a problem hiding this comment.
@yxxhero What do you mean by adding notes? In the readme?
My assumption was, that no one was actually using the secretserver provider yet, since I added it just before Christmas. To bad there already is a release containing the longer name.
There was a problem hiding this comment.
@pd-gov you are right. so we can use new name. I will add the changes in the release notes.
There was a problem hiding this comment.
@yxxhero The current version contains your proposed aliases. Should I keep that in for backwards compatibility?
There was a problem hiding this comment.
@yxxhero Okay, I removed the alias again.
pd-gov
force-pushed
the
feature/secretserver
branch
from
197c554 to
752ec95
Compare
Signed-off-by: Dargel, Philipp <philipp.dargel@governikus.de>
pd-gov
force-pushed
the
feature/secretserver
branch
from
752ec95 to
fbc43b7
Compare
I discovered that the vendor provides an official library. So I replaced the raw http implementation with that.
Additional changes:
tssin code and environment variables.The changes are tested against an on-prem instance. The library also allows easier use with their cloud offering. Support is implemented but could not be tested.